ncm-afsclt: fix handling of size AUTOMATIC #816

jouvin · 2016-07-15T14:50:08Z

Fixes #815.

jouvin · 2016-07-15T14:51:05Z

I'll try to add a unit test. This is one weakness of this configuration module: there is basically no unit test.

jrha · 2016-07-15T14:52:41Z

That and many sites have stopped using AFS.

jouvin · 2016-07-15T16:03:02Z

Unit tests added for test configurations. Uncovered a few potential issues, in particular if the initial cacheinfo file was empty.

stdweird · 2016-07-15T17:47:55Z

ncm-afsclt/src/main/perl/afsclt.pm

-    $afs_cacheinfo_fh->cancel();
-    if ( "$afs_cacheinfo_fh" =~ m;^([^:]+):([^:]+):(\d+)$; ) {
+    my $afs_cacheinfo_fh = CAF::FileReader->new( $AFS_CACHEINFO, log => $self );
+    $self->debug(2, "$AFS_CACHEINFO current contents: >>>$afs_cacheinfo_fh<<<");


only in else block? (and in that case, maybe in the error message)

In fact, I realized that my mod was wrong because if was no longer possible to override value in cacheinfo by the Quattor configuration. cacheinfo should only be used as default values. fixed.

stdweird · 2016-07-15T17:58:35Z

some minor remarks, LGTM otherwise

jouvin · 2016-07-15T18:39:52Z

@stdweird thanks as usual for the remarks. I'll address them tomorrow. Most of them in fact apply to the original code and its last modification in december. I tried to address an urgent problem but didn't attempt to fix all the possible ones. I'll also try to improve a little bit the unit tests.

jouvin · 2016-07-16T08:15:51Z

Should be ok now. Remarks addresssed. Unit tests improved. cacheinfo values used only as default values.

jouvin · 2016-07-16T09:00:44Z

@ajf8 you may want to have a look and to check that this doesn't break anything for you, compared to the last modifications you committed in December 2015.

stdweird · 2016-07-16T09:12:34Z

ncm-afsclt/src/main/pan/components/afsclt/schema.pan

+   "afs_mount"  ? string         # AFS mount point (e.g. /afs)
+   "cachemount" ? string         # AFS cache location (/usr/vice/etc/cache)
+   "cachesize"  ? string         # AFS cache size in kB
+   "enabled"    : string with match (SELF, 'yes|no') # Shall AFS client be active ?


use legacy_binary_affirmation_string instead.

stdweird · 2016-07-16T09:18:24Z

@jouvin thanks for the further cleanup.

…atures

- Also type cleanups

- Previously tending to return a status only in case of error...

stdweird · 2016-07-17T08:01:26Z

ncm-afsclt/src/main/perl/afsclt.pm

+        $self->debug(1, "AFS mount point not defined: using default ($AFS_MOUNTPOINT_DEF)");
+        $file_afsmount = $AFS_MOUNTPOINT_DEF;
+    }
+    if ( "$new_cache" ne "$file_cache" ) {


with this check, the AFS_CACHEINIFO is only updated is file_cache is modified, nit file_afsmount or file_cache_mount
i would always write the file, and with the close returns a change in content, do the block below.

Good catch!

- Also fix return status in case of success in Configure_CellServDB (used to be 1 instead of 0)

…hanged

jouvin · 2016-07-17T14:32:12Z

As for me, ready for merging! @stdweird any other remark?!

stdweird · 2016-07-17T15:11:40Z

ncm-afsclt/src/main/perl/afsclt.pm

    my $thiscell_fh = CAF::FileWriter->new( $THISCELL, log => $self );
-    print $thiscell_fh "$afscell\n";
+    print $thiscell_fh $config->{thiscell}."\n";


there's no need for the stirng concat, "$config->{thiscell}\n" should also work, now that $config is a hashref.

stdweird · 2016-07-17T15:19:31Z

@jouvin LGTM (and 👍 for further cleanup and tests)
@jrha not sure if we should wait for feedback from @ajf8 , esp if this is for 16.6.

jouvin · 2016-07-17T17:34:42Z

@ajf8 feedback would be great but I really think I introduced no feature changes but just fixed things that were addressing just the MS way of configuring AFS but had some flaws due to the lack of unit testing (the main change introduced by this PR!).

ned21 · 2016-07-17T19:14:42Z

ncm-afsclt/src/main/pan/components/afsclt/schema.pan

+   "afs_mount"  ? string         # AFS mount point (e.g. /afs)
+   "cachemount" ? string         # AFS cache location (/usr/vice/etc/cache)
+   "cachesize"  ? string         # AFS cache size in kB
+   "enabled"    : legacy_binary_affirmation_string  # Shall AFS client be active ?


IMHO this is a retrograde step for readability. I now need to locate this type defined in some other file before I can understand how to populate this mandatory field, which has no default. legacy_yes_or_no_string would be more intuitive.

I tend to agree... I was thinking the same thing when doing the modification suggested by @stdweird ...

Feel free to open an issue to change. This is work done in quattor/template-library-core#111 and #749; but renaming the type should be very trivial.

Done. quattor/template-library-core#118.

IMHO it would be better to make the effort to change the type to boolean instead of renaming the legacy type.

ajf8 · 2016-07-18T10:11:16Z

Thanks for the improvements @jouvin and sorry about the break. I think this module needed some work, hopefully I got some of it done, but it looks much better now especially with the unit tests.

Please feel free to merge, I will test it sometime and submit a new pull request if anything is needed.

stdweird · 2016-07-18T10:15:52Z

@ajf8 thanks for the feedback

ncm-afsclt: fix handling of size AUTOMATIC

4cb5a41

Fixes quattor#815.

jouvin added this to the 16.6 milestone Jul 15, 2016

jouvin self-assigned this Jul 15, 2016

stdweird reviewed Jul 15, 2016
View reviewed changes

jouvin force-pushed the afsclt branch from 9520225 to 73e052f Compare July 16, 2016 07:08

ncm-afsclt: allow initial cacheinfo file to be empty

56af89a

jouvin force-pushed the afsclt branch from 73e052f to 01f4e91 Compare July 16, 2016 07:16

jouvin added 2 commits July 16, 2016 09:31

ncm-afsclt: allow to define AFS mount point

7c72faa

ncm-afsclt: misc. change in schema to allow to unit test it

845fa03

jouvin force-pushed the afsclt branch from 01f4e91 to 9205f3f Compare July 16, 2016 08:14

stdweird reviewed Jul 16, 2016
View reviewed changes

jouvin force-pushed the afsclt branch from fb5b75d to 669f83b Compare July 16, 2016 14:37

jouvin added 4 commits July 16, 2016 16:49

ncm-afsclt: add unit tests for cache configuration

f508894

ncm-afsclt: update documentation to reflect schema and implemented fe…

d5f71bc

…atures

ncm-afsclt: remove 'options' from schema (not managed by code)

fb49435

- Also type cleanups

ncm-afsclt: add myself as a developer of afsclt

5be2b85

jouvin added 2 commits July 16, 2016 16:49

ncm-afsclt: add unit tests for Configure_Cell

c8cb6a4

ncm-afsclt: ensure that all methods return a status

209704a

- Previously tending to return a status only in case of error...

jouvin force-pushed the afsclt branch from 669f83b to 209704a Compare July 16, 2016 14:49

jouvin added 3 commits July 16, 2016 19:18

ncm-afsclt: minor reformatting

79d39f2

ncm-afsclt: add unit tests for Configure_TheseCells

c7fae1d

ncm-afsclt: add unit tests for Configure_Afsd_Args

a546dd8

jouvin force-pushed the afsclt branch from f9594a0 to a546dd8 Compare July 16, 2016 18:29

stdweird reviewed Jul 17, 2016
View reviewed changes

jouvin added 4 commits July 17, 2016 12:32

ncm-afsclt: add unit tests for Configure_CellServDB

31e8126

- Also fix return status in case of success in Configure_CellServDB (used to be 1 instead of 0)

ncm-afsclt: use getTree() API instead of the legacy one

5c15bd3

ncm-afsclt: refactoring of unit tests

086142f

ncm-afsclt: ensure cacheinfo file is updated whatever the parameter c…

f9ce48f

…hanged

stdweird reviewed Jul 17, 2016
View reviewed changes

ncm-afsclt: remove unnecessary string concat in print statements

f831796

ned21 reviewed Jul 17, 2016
View reviewed changes

jouvin mentioned this pull request Jul 18, 2016

Change type legacy_binary_affirmation_string to legacy_yes_or_no_string quattor/template-library-core#118

Open

stdweird merged commit e57244a into quattor:master Jul 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ncm-afsclt: fix handling of size AUTOMATIC #816

ncm-afsclt: fix handling of size AUTOMATIC #816

jouvin commented Jul 15, 2016

jouvin commented Jul 15, 2016

jrha commented Jul 15, 2016

jouvin commented Jul 15, 2016

stdweird Jul 15, 2016

jouvin Jul 16, 2016

stdweird commented Jul 15, 2016

jouvin commented Jul 15, 2016

jouvin commented Jul 16, 2016

jouvin commented Jul 16, 2016

stdweird Jul 16, 2016

stdweird commented Jul 16, 2016

stdweird Jul 17, 2016

jouvin Jul 17, 2016

jouvin commented Jul 17, 2016

stdweird Jul 17, 2016

stdweird commented Jul 17, 2016

jouvin commented Jul 17, 2016

ned21 Jul 17, 2016

jouvin Jul 17, 2016

stdweird Jul 18, 2016

jouvin Jul 18, 2016

jrha Jul 18, 2016

ajf8 commented Jul 18, 2016

stdweird commented Jul 18, 2016

ncm-afsclt: fix handling of size AUTOMATIC #816

ncm-afsclt: fix handling of size AUTOMATIC #816

Conversation

jouvin commented Jul 15, 2016

jouvin commented Jul 15, 2016

jrha commented Jul 15, 2016

jouvin commented Jul 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdweird commented Jul 15, 2016

jouvin commented Jul 15, 2016

jouvin commented Jul 16, 2016

jouvin commented Jul 16, 2016

Choose a reason for hiding this comment

stdweird commented Jul 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jouvin commented Jul 17, 2016

Choose a reason for hiding this comment

stdweird commented Jul 17, 2016

jouvin commented Jul 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajf8 commented Jul 18, 2016

stdweird commented Jul 18, 2016